Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swagger Integration #240

Merged
merged 41 commits into from
Oct 24, 2023
Merged

Swagger Integration #240

merged 41 commits into from
Oct 24, 2023

Conversation

LeaveNhA
Copy link
Contributor

Title: Swagger Integration

Description

In this PR, simply aimed to implement the Swagger Integration in Xiana-way. I'm fully open to critics

Tester info

The tests are so naive, so let me write more tests.

Completion Checklist

Nothing, now.

Comment on lines 10 to 18
(-> routes
helpers/routes->routes'
((if (not (false? render?))
#(helpers/routes->swagger-data % :route-opt-map route-opt-map)
identity))
((cond
(= type :json) json/write-value-as-string
(= type :edn) identity
:else identity))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I was meant to write actually. If you don't have any strong opinions, I would rather go with this way on that.

type :type
route-opt-map :route-opt-map}]
(if (swagger-configs-there? config)
(let [routes (or routes (-> config :routes))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with more accurate version, get.

Comment on lines 43 to 45
config-key (-> config
(get :xiana/swagger {})
(get :path :swagger.json))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config-key (-> config
(get :xiana/swagger {})
(get :path :swagger.json))]
config-key (get-in config [:xiana/swagger :path] :swagger.json)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was helpful but evantually, it will grow like this because of the potential of the BL.
Also, fixed, for now.

Comment on lines 26 to 29
(->> config
((juxt :xiana/swagger-ui :xiana/swagger))
(reduce #(and % %2) true)
boolean))
Copy link
Collaborator

@g-krisztian g-krisztian Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(apply and ((juxt :xiana/swagger-ui :xiana/swagger) config))

or

(every? some? ((juxt :xiana/swagger-ui :xiana/swagger) config))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(apply and ((juxt :xiana/swagger-ui :xiana/swagger) config)) is the right reflection of that s-expression without overseeing any future changes. Fixed, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, throwing an exception due to and is a macro. Going with the second option for a while. Thanks.

@@ -16,3 +23,73 @@
(assoc :error e)
(assoc :response
{:status 500 :body "Internal Server error"})))))

(defonce all-methods
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be part of xiana.swagger ns, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was seeing the future again, and experimenting with more advanced and growth code-base. So, for now, fixed.

@g-krisztian
Copy link
Collaborator

I'd like to see the default swagger endpoints added to the app routes on route initialization at https://github.com/Flexiana/framework/blob/main/src/xiana/route.clj#L11 when the swagger feature is enabled in configuration.

@LeaveNhA
Copy link
Contributor Author

LeaveNhA commented Oct 25, 2022

I'd like to see the default swagger endpoints added to the app routes on route initialization at https://github.com/Flexiana/framework/blob/main/src/xiana/route.clj#L11 when the swagger feature is enabled in configuration.

I was thinking on that too, but you are still ignoring the people who might want to configure or tweak their enpoints. I think we can give them some configuration options by our configuration system but if you think carefully there is still some things that we can't configure with this way. For now, we provide basic functionality also, less configuration. Let me finish and push it. So we can examine it.

Edit: your point is adding them silently, only by configuration, I believe. It seems not right to me. I would love to hear different perspectives over it, first.

@LeaveNhA LeaveNhA self-assigned this Oct 27, 2022
@LeaveNhA LeaveNhA requested a review from g-krisztian October 28, 2022 10:02
@LeaveNhA LeaveNhA mentioned this pull request Nov 4, 2022
src/xiana/swagger.clj Outdated Show resolved Hide resolved
@gmsvalente gmsvalente force-pushed the dev/swagger-integration branch from ece3066 to 7d2f9db Compare October 5, 2023 17:27
@soulflyer soulflyer requested review from agigao and g-krisztian and removed request for agigao, narocath and g-krisztian October 20, 2023 11:40
@soulflyer soulflyer merged commit 080d0e2 into main Oct 24, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants